-
Notifications
You must be signed in to change notification settings - Fork 871
[Doc Generation] Support parsing posonlyargs when get function signature
#7578
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
感谢你贡献飞桨文档,文档预览构建中,Docs-New 跑完后即可预览,预览链接:http://preview-pr-7578.paddle-docs-preview.paddlepaddle.org.cn/documentation/docs/zh/api/index_cn.html |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a bug in the API documentation generation script that caused an IndexError when parsing Python functions with position-only arguments (posonlyargs). The issue occurred when processing functions like register_fake in paddle/library.py that only had posonlyargs and kwonlyargs without regular args.
Key Changes:
- Added parsing logic for position-only arguments (posonlyargs)
- Updated default value assignment logic to handle defaults for posonlyargs
- Added logic to skip the "/" separator when assigning default values
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
docs/api/gen_doc.py
Outdated
|
|
||
| for defarg_ind in range(len(node.args.defaults)): | ||
| if str_args_list[defarg_ind_start + defarg_ind] == "/": | ||
| defarg_ind_start += 1 |
Copilot
AI
Oct 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check is inside the loop and modifies defarg_ind_start each iteration, which can cause incorrect indexing for subsequent defaults. The slash skip should only occur once when first encountered. Consider moving this check outside the loop or using a flag to ensure it only executes once.
| for defarg_ind in range(len(node.args.defaults)): | |
| if str_args_list[defarg_ind_start + defarg_ind] == "/": | |
| defarg_ind_start += 1 | |
| # Skip the "/" marker only once if it is at the start index | |
| if ( | |
| defarg_ind_start < len(str_args_list) | |
| and str_args_list[defarg_ind_start] == "/" | |
| ): | |
| defarg_ind_start += 1 | |
| for defarg_ind in range(len(node.args.defaults)): |
docs/api/gen_doc.py
Outdated
| defarg_ind_start -= 1 | ||
|
|
||
| for defarg_ind in range(len(node.args.defaults)): | ||
| if str_args_list[defarg_ind_start + defarg_ind] == "/": |
Copilot
AI
Oct 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential IndexError if defarg_ind_start + defarg_ind exceeds the list bounds. This can occur when there are more defaults than combined posonlyargs and args. Add a bounds check before accessing the list element.
docs/api/gen_doc.py
Outdated
|
|
||
| defarg_ind_start = len(str_args_list) - len(node.args.defaults) | ||
| if node.args.posonlyargs is not None and len(node.args.defaults) > len( | ||
| node.args.args | ||
| ): |
Copilot
AI
Oct 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The calculation of defarg_ind_start adjustment is unclear. The logic assumes defaults overflow into posonlyargs territory by exactly one position, but this may not account for all edge cases. Consider adding a comment explaining the mathematical relationship between defaults, args, posonlyargs, and the '/' separator position.
| defarg_ind_start = len(str_args_list) - len(node.args.defaults) | |
| if node.args.posonlyargs is not None and len(node.args.defaults) > len( | |
| node.args.args | |
| ): | |
| # Calculate the starting index in str_args_list where default values should be applied. | |
| # The list str_args_list contains argument names in the order: posonlyargs (if any), '/', args. | |
| # node.args.defaults contains default values for the last N positional arguments (posonlyargs + args). | |
| # The defaults are applied to the last N arguments, which may include both args and posonlyargs. | |
| # If the number of defaults exceeds the number of args, the extra defaults apply to posonlyargs. | |
| # The '/' separator is present if posonlyargs exist, and must be skipped when assigning defaults. | |
| defarg_ind_start = len(str_args_list) - len(node.args.defaults) | |
| if node.args.posonlyargs is not None and len(node.args.defaults) > len( | |
| node.args.args | |
| ): | |
| # If defaults overflow into posonlyargs, adjust the start index to skip the '/' separator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@SigureMo 一师傅要不要看看,提点建议
|
docs/api/gen_doc.py
Outdated
| logger.debug("%s omitted", obj_full_name) | ||
|
|
||
|
|
||
| def gen_functions_args_str(node): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def gen_function_args_string(fn_node: ast.FunctionDef) -> str:
def _gen_default_value_string(default_node: ast.expr) -> str:
if isinstance(default_node, ast.Constant):
default_value = default_node.value
if isinstance(default_value, str):
return f"'{default_value}'"
return str(default_value)
elif isinstance(default_node, ast.Name):
return default_node.id
return "..."
def _is_self_arg(arg: ast.arg) -> bool:
return arg.arg == "self"
if not isinstance(fn_node, ast.FunctionDef):
warnings.warn(f"The provided node {ast.unparse(fn_node)} is not a FunctionDef.")
return ""
arg_list = []
posonlyargs_count = len(fn_node.args.posonlyargs)
for i, arg in enumerate(fn_node.args.posonlyargs):
if _is_self_arg(arg):
continue
arg_list.append(arg.arg)
if posonlyargs_count > 0:
arg_list.append("/")
for i, arg in enumerate(fn_node.args.args):
if _is_self_arg(arg):
continue
arg_list.append(arg.arg)
defaults_start_index = len(fn_node.args.args) - len(fn_node.args.defaults)
for i, default in enumerate(fn_node.args.defaults):
arg_index = defaults_start_index + i
arg_name = fn_node.args.args[arg_index].arg
default_value_str = _gen_default_value_string(default)
arg_list[arg_index] = f"{arg_name}={default_value_str}"
if fn_node.args.vararg:
arg_list.append(f"*{fn_node.args.vararg.arg}")
if fn_node.args.kwonlyargs:
arg_list.append("*")
for i, kwonlyarg in enumerate(fn_node.args.kwonlyargs):
kwdefault = fn_node.args.kw_defaults[i]
assert kwdefault is not None
default_value_str = _gen_default_value_string(kwdefault)
arg_list.append(f"{kwonlyarg.arg}={default_value_str}")
if fn_node.args.kwarg:
arg_list.append(f"**{fn_node.args.kwarg.arg}")
return ", ".join(arg_list)直接重写一下吧
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defaults_start_index = len(fn_node.args.args) - len(fn_node.args.defaults)
这个好像不对吧,当时那个解析出错的函数没有args,但存在posonlyargs,然后默认值是分配给posonlyargs的
所以defaults_start_index不但需要考虑args,还需要考虑posonlyargs和后面那个斜杠参数
最新的commit已针对此问题进行调整
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defaults_start_index = (
len(fn_node.args.args)
+ len(fn_node.args.posonlyargs)
- len(fn_node.args.defaults)
)
for i, arg in enumerate(fn_node.args.posonlyargs + fn_node.args.args):
if _is_self_arg(arg) and i == 0:
continue
if i == posonlyargs_count and posonlyargs_count > 0:
arg_list.append("/")
if i >= defaults_start_index:
default_value_str = _gen_default_value_str(
fn_node.args.defaults[i - defaults_start_index]
)
arg_list.append(f"{arg.arg}={default_value_str}")
else:
arg_list.append(arg.arg)那就这么写
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个也改好了
|
Docs-NEW报了一个AssertionError,发现是 def make_scheduler(
*,
closed: int,
ready: int,
record: int,
repeat: int = 0,
skip_first: int = 0,
) -> Callable[[int], ProfilerState]并不是所有的kwonlyargs都分配了默认值,至于为什么原先的代码能正确识别默认值,是因为这个函数的 [None, None, None, <ast.Constant object at 0x711b5545f450>, <ast.Constant object at 0x711b5545f350>] |
docs/api/gen_doc.py
Outdated
| for i, kwonlyarg in enumerate(fn_node.args.kwonlyargs): | ||
| kwdefault = fn_node.args.kw_defaults[i] | ||
| assert kwdefault is not None | ||
| default_value_str = _gen_default_value_string(kwdefault) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
不是直接 remove assert 就行的
if kwdefault is None:
arg_list.append(kwonlyarg.arg)
else:
default_value_str = _gen_default_value_string(kwdefault)
arg_list.append(f"{kwonlyarg.arg}={default_value_str}")There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
改好了,前面看太急了,不好意思喵
posonlyargs when get function signature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR types
others
PR changes
Docs
Description
issue #7435
【详细报告】
在调查8号文档产生问题的过程中,发现英文文档生成过程报错,报错后仍然继续进行build
以下内容摘自Docs-NEW流水线的日志
排查发现,产生报错的文件为Paddle仓库的
python/paddle/library.py文件错误为函数解析错误,产生错误的函数是
custom_op和register_fake以下为
register_fake函数的声明该函数只有posonlyargs和kwonlyargs,没有普通的args,而
gen_doc.py中并没有对posonlyargs进行处理在分配参数默认值defaults时,实际str_args_list为空,目标位置为
-1,因此报错本次修改新增了posonlyargs的处理逻辑
更新了分配参数默认值的逻辑
已确认该报错在Docs-NEW运行中不再出现,然而此问题并非导致
paddle.static.Program英文文档渲染缺少内容的原因,后续会继续调查